fix: datetime for defaultLocale set fa #238

datamweb · 2022-06-12T17:02:49Z

When toDateTimeString() is used. If defaultLocale = 'fa'.
The value of the datetime will be UTF-8 string (33) "۲۰۲۲-۰۶-۱۲ ۲۲:۳۰:۳۵".
In this case, the database enters the information as 0000-00-00 00:0:00.
Which causes a bug in the shield.
In this PR I tried to solve this problem.

note: This method can also solve the problem. $time = new Time('now','','en-US')

Data of Table "auth_groups_users":

+----+---------+-------+--------------------+
| id | user_id | group | created_at         |
+----+---------+-------+--------------------+
| 7  | 7       | user  | 0000-00-00 00:0... |
| 22 | 22      | user  | 0000-00-00 00:0... |
| 23 | 23      | user  | 2022-06-12 19:3... |
+----+---------+-------+--------------------+

datamweb · 2022-06-12T17:05:26Z

@kenjis I'm not sure this is the best solution, please review.
I will also send the test after confirmation.

kenjis · 2022-06-12T21:53:51Z

toDateTimeString() returns localized datetime string. So it is not appropriate.

Your solution is good.

datamweb · 2022-06-13T07:09:41Z

I tried to put the test in the appropriate file. Adding this test to other files(AccessTokens.php , MagicLinkController.php ...) seems to cause bloating.
However please let me know if you need to change.

tests/Authorization/AuthorizableTest.php

Co-authored-by: kenjis <kenji.uui@gmail.com>

tests/Authorization/AuthorizableTest.php

Co-authored-by: kenjis <kenji.uui@gmail.com>

kenjis · 2022-06-13T08:53:07Z

tests/Authorization/AuthorizableTest.php

+ $this->dontSeeInDatabase('auth_groups_users', [
+ 'user_id' => $this->user->id,
+ 'group' => 'admin',
+ 'created_at' => '0000-00-00 00:00:00',


Isn't '0000-00-00 00:00:00' MySQL specific?

Does this test fail before applying your fixes?

kenjis · 2022-06-13T09:07:21Z

tests/Authorization/AuthorizableTest.php

+ $this->hasInDatabase('auth_groups_users', [
+ 'user_id' => $this->user->id,
+ 'group' => 'admin',
+ 'created_at' => Time::now()->format('Y-m-d H:i:s'),
+ ]);


hasInDatabase() inserts a record:
https://codeigniter4.github.io/CodeIgniter4/testing/database.html?highlight=hasindatabase#hasindatabase-table-data
Why do you need to insert the data here?

In fact, I was just trying to check the correct record of the date in the database.
That's why I used it.

The test does not check anything.

Because hasInDatabase() inserts a record.
So the admin record for the user exists.
So $this->user->addGroup('admin') does not insert a record.

After all, you insert a record by hasInDatabase() and check it by dontSeeInDatabase().
You don't check any product code behavior.

Oh my God.
This is a ridiculous mistake, I'm sorry.
Thanks for the explanation.

Not your fault @datamweb, I have always felt that hasInDatabse() was very poorly named.

kenjis · 2022-06-13T09:30:48Z

@datamweb The following test should work, but it does not work because of CI4 Time bug. codeigniter4/CodeIgniter4#6116

    public function testCreatedAtIfDefaultLocaleSetFaWithAddGroup(): void
    {
        $currentLocale = Locale::getDefault();
        Locale::setDefault('fa');

        Time::setTestNow('March 10, 2017', 'America/Chicago');

        $this->user->addGroup('admin');

        $this->seeInDatabase('auth_groups_users', [
            'user_id'    => $this->user->id,
            'group'      => 'admin',
            'created_at' => '2017-03-10 00:00:00',
        ]);

        Locale::setDefault($currentLocale);
    }

datamweb · 2022-06-13T09:43:02Z

    public function testCreatedAtIfDefaultLocaleSetFaWithAddGroup(): void
    {
        $currentLocale = Locale::getDefault();
        Locale::setDefault('fa');

        Time::setTestNow('March 10, 2017', 'America/Chicago');

        $this->user->addGroup('admin');

        $this->seeInDatabase('auth_groups_users', [
            'user_id'    => $this->user->id,
            'group'      => 'admin',
            'created_at' => '2017-03-10 00:00:00',
        ]);

        Locale::setDefault($currentLocale);
    }

@kenjis, So should I replace the test?

MGatner · 2022-06-13T11:35:51Z

Let's hold off on this until we resolve the issue with the framework. The User Guide seems strongly in favor of toDateTimeString() for database inserts, so this might be the correct code and the bug is actually that the framework is localizing the result.

datamweb · 2022-06-13T12:05:59Z

Let's hold off on this until we resolve the issue with the framework. The User Guide seems strongly in favor of toDateTimeString() for database inserts, so this might be the correct code and the bug is actually that the framework is localizing the result.

Okay. Please let me know if need to do something.

kenjis · 2022-06-15T08:35:14Z

@MGatner The bug in the framework was fixed in the develop, but we can't use it in this repository until it is released.
What do we do?

MGatner · 2022-06-15T10:25:31Z

This PR is still good, it is just the test case that needs the framework change, correct? Since the test case is covering an external bug I don't think it is necessary - we can proceed with the content change. If you want to leave the test in place with a conditional skip it could check the framework version on CodeIgniter\CodeIgniter.

MGatner · 2022-06-15T10:26:47Z

... Alternatively I can make the bugfix release of 4.2.1 today and we can use that.

kenjis · 2022-06-15T12:28:57Z

Yes, the product code seems okay, but test code is now broken.
And if we write correct test, we hit the framework bug.

If you release 4.2.1, it might be better to revert codeigniter4/CodeIgniter4#5653
See codeigniter4/CodeIgniter4#6086 (comment)

kenjis · 2022-06-16T02:41:22Z

@datamweb Update the test, and let's move on.

--- a/tests/Authorization/AuthorizableTest.php
+++ b/tests/Authorization/AuthorizableTest.php
@@ -2,6 +2,7 @@
 
 namespace Tests\Authorization;
 
+use CodeIgniter\CodeIgniter;
 use CodeIgniter\I18n\Time;
 use CodeIgniter\Shield\Authorization\AuthorizationException;
 use CodeIgniter\Shield\Models\UserModel;
@@ -302,22 +303,23 @@ public function testCanCascadesToGroupsWithWildcards(): void
      */
     public function testCreatedAtIfDefaultLocaleSetFaWithAddGroup(): void
     {
+        if (version_compare(CodeIgniter::CI_VERSION, '4.2.1', '<')) {
+            $this->markTestSkipped('This test does not work on CI v4.2.0 or before due to a bug.');
+        }
+
         $currentLocale = Locale::getDefault();
         Locale::setDefault('fa');
 
-        $this->hasInDatabase('auth_groups_users', [
-            'user_id'    => $this->user->id,
-            'group'      => 'admin',
-            'created_at' => Time::now()->format('Y-m-d H:i:s'),
-        ]);
+        Time::setTestNow('March 10, 2017', 'America/Chicago');
 
         $this->user->addGroup('admin');
 
-        $this->dontSeeInDatabase('auth_groups_users', [
+        $this->seeInDatabase('auth_groups_users', [
             'user_id'    => $this->user->id,
             'group'      => 'admin',
-            'created_at' => '0000-00-00 00:00:00',
+            'created_at' => '2017-03-10 00:00:00',
         ]);
+
         Locale::setDefault($currentLocale);
     }
 }

MGatner

PHPStan doesn't like the constant being variable. I don't think this will fix Rector though, we probably need to add a skip for it.

I will try to release 4.2.1 this morning so maybe we can ignore all this.

tests/Authorization/AuthorizableTest.php

Co-authored-by: MGatner <mgatner@icloud.com>

kenjis · 2022-06-16T12:27:04Z

--- a/rector.php
+++ b/rector.php
@@ -20,6 +20,7 @@
 use Rector\Config\RectorConfig;
 use Rector\Core\ValueObject\PhpVersion;
 use Rector\DeadCode\Rector\ClassMethod\RemoveUnusedPromotedPropertyRector;
+use Rector\DeadCode\Rector\If_\UnwrapFutureCompatibleIfPhpVersionRector;
 use Rector\DeadCode\Rector\MethodCall\RemoveEmptyMethodCallRector;
 use Rector\DeadCode\Rector\StmtsAwareInterface\RemoveJustPropertyFetchForAssignRector;
 use Rector\EarlyReturn\Rector\Foreach_\ChangeNestedForeachIfsToEarlyContinueRector;
@@ -80,6 +81,11 @@
             __DIR__ . '/tests',
         ],
 
+        // check on constant compare
+        UnwrapFutureCompatibleIfPhpVersionRector::class => [
+            __DIR__ . '/tests/Authorization/AuthorizableTest.php',
+        ],
+
         // Ignore files that should not be namespaced to their folder
         NormalizeNamespaceByPSR4ComposerAutoloadRector::class => [
             __DIR__ . '/src/Helpers',
diff --git a/tests/Authorization/AuthorizableTest.php b/tests/Authorization/AuthorizableTest.php
index 3667f7c..0a8ea43 100644
--- a/tests/Authorization/AuthorizableTest.php
+++ b/tests/Authorization/AuthorizableTest.php
@@ -308,6 +308,7 @@ public function testCreatedAtIfDefaultLocaleSetFaWithAddGroup(): void
             $this->markTestSkipped('This test does not work on CI v4.2.0 or before due to a bug.');
         }
 
+        // @phpstan-ignore-next-line
         $currentLocale = Locale::getDefault();
         Locale::setDefault('fa');

MGatner · 2022-06-16T23:38:51Z

4.2.1 is out, safe to roll back these workarounds

tests/Authorization/AuthorizableTest.php

kenjis · 2022-06-26T02:59:53Z

@datamweb Removing the if in the test case is one last job for this PR.
Can you?

Co-authored-by: kenjis <kenji.uui@gmail.com>

datamweb · 2022-06-28T18:54:26Z

Hi @kenjis , I'm sorry for the delay. I was busy these few days.
Thanks for the review.

kenjis · 2022-06-28T22:08:14Z

Okay, all green!
Merged.

kenjis added the bug Something isn't working label Jun 12, 2022

datamweb marked this pull request as draft June 13, 2022 01:55

datamweb changed the title ~~fix: datatime for defaultLocale set fa~~ fix: datetime for defaultLocale set fa Jun 13, 2022

fix: datetime for defaultLocale set fa

70e81cc

datamweb force-pushed the fix-datatime-if-defaultLocale-fa branch from e6f7014 to 70e81cc Compare June 13, 2022 05:58

test: add test if default locale set fa

77981d9

datamweb marked this pull request as ready for review June 13, 2022 07:10

kenjis reviewed Jun 13, 2022

View reviewed changes

tests/Authorization/AuthorizableTest.php Outdated Show resolved Hide resolved

update PHPDoc

f4e567a

Co-authored-by: kenjis <kenji.uui@gmail.com>

kenjis reviewed Jun 13, 2022

View reviewed changes

tests/Authorization/AuthorizableTest.php Outdated Show resolved Hide resolved

kenjis reviewed Jun 13, 2022

View reviewed changes

tests/Authorization/AuthorizableTest.php Show resolved Hide resolved

datamweb and others added 2 commits June 13, 2022 13:09

Update tests/Authorization/AuthorizableTest.php

2bd7765

Co-authored-by: kenjis <kenji.uui@gmail.com>

Update AuthorizableTest.php

7c1138e

kenjis reviewed Jun 13, 2022

View reviewed changes

test: Edit test according to CI version

79d3e3e

MGatner requested changes Jun 16, 2022

View reviewed changes

tests/Authorization/AuthorizableTest.php Outdated Show resolved Hide resolved

Update tests/Authorization/AuthorizableTest.php

1b0e643

Co-authored-by: MGatner <mgatner@icloud.com>

kenjis reviewed Jun 26, 2022

View reviewed changes

tests/Authorization/AuthorizableTest.php Outdated Show resolved Hide resolved

datamweb and others added 2 commits June 28, 2022 23:14

Update tests/Authorization/AuthorizableTest.php

2cf38c6

Co-authored-by: kenjis <kenji.uui@gmail.com>

Update AuthorizableTest.php

e929fc8

kenjis merged commit 0dd0562 into codeigniter4:develop Jun 28, 2022

datamweb deleted the fix-datatime-if-defaultLocale-fa branch June 28, 2022 22:43

kenjis mentioned this pull request Aug 26, 2022

fix: correctly record the datetime for last_active #404

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: datetime for defaultLocale set fa #238

fix: datetime for defaultLocale set fa #238

datamweb commented Jun 12, 2022 •

edited

Loading

datamweb commented Jun 12, 2022

kenjis commented Jun 12, 2022

datamweb commented Jun 13, 2022

kenjis Jun 13, 2022

kenjis Jun 13, 2022

datamweb Jun 13, 2022

kenjis Jun 13, 2022 •

edited

Loading

datamweb Jun 13, 2022

MGatner Jun 13, 2022

kenjis commented Jun 13, 2022

datamweb commented Jun 13, 2022

MGatner commented Jun 13, 2022

datamweb commented Jun 13, 2022 •

edited

Loading

kenjis commented Jun 15, 2022

MGatner commented Jun 15, 2022

MGatner commented Jun 15, 2022

kenjis commented Jun 15, 2022

kenjis commented Jun 16, 2022

MGatner left a comment •

edited

Loading

kenjis commented Jun 16, 2022

MGatner commented Jun 16, 2022

kenjis commented Jun 26, 2022

datamweb commented Jun 28, 2022

kenjis commented Jun 28, 2022

fix: datetime for defaultLocale set fa #238

fix: datetime for defaultLocale set fa #238

Conversation

datamweb commented Jun 12, 2022 • edited Loading

datamweb commented Jun 12, 2022

kenjis commented Jun 12, 2022

datamweb commented Jun 13, 2022

kenjis Jun 13, 2022

Choose a reason for hiding this comment

kenjis Jun 13, 2022

Choose a reason for hiding this comment

datamweb Jun 13, 2022

Choose a reason for hiding this comment

kenjis Jun 13, 2022 • edited Loading

Choose a reason for hiding this comment

datamweb Jun 13, 2022

Choose a reason for hiding this comment

MGatner Jun 13, 2022

Choose a reason for hiding this comment

kenjis commented Jun 13, 2022

datamweb commented Jun 13, 2022

MGatner commented Jun 13, 2022

datamweb commented Jun 13, 2022 • edited Loading

kenjis commented Jun 15, 2022

MGatner commented Jun 15, 2022

MGatner commented Jun 15, 2022

kenjis commented Jun 15, 2022

kenjis commented Jun 16, 2022

MGatner left a comment • edited Loading

Choose a reason for hiding this comment

kenjis commented Jun 16, 2022

MGatner commented Jun 16, 2022

kenjis commented Jun 26, 2022

datamweb commented Jun 28, 2022

kenjis commented Jun 28, 2022

datamweb commented Jun 12, 2022 •

edited

Loading

kenjis Jun 13, 2022 •

edited

Loading

datamweb commented Jun 13, 2022 •

edited

Loading

MGatner left a comment •

edited

Loading